[hotfix][Optimizer] Recognize CLOSED optimizing processes - #4331
Conversation
close() sets ProcessStatus.CLOSED while isClosed() only compared against KILLED - a status never assigned in this class - so the predicate was permanently false and acceptResult's OptimizingClosedException guard against late results on a closed process could never fire (downstream isProcessing conditions happened to contain the fallout). Check both CLOSED and KILLED. Reachability analysis in the fix record: close(false) cancels tasks so late completes fail earlier with IllegalTaskStateException, and the partial-commit path legitimately accepts late results; no public-API path drives the guard today, so this lands as an intent fix verified by the full queue regression (47/47) rather than a behavioral red test. Fix record: docs/fix-records/2026-08-16-fix-22-is-closed-status-mismatch.md
65aadac to
96a97b2
Compare
| @@ -775,7 +775,10 @@ private void resetTask(TaskRuntime<RewriteStageTask> taskRuntime) { | |||
|
|
|||
| @Override | |||
| public boolean isClosed() { | |||
There was a problem hiding this comment.
I'm wondering why we need isClosed() if we already have getStatus(). If it represents something other than the process status, we should either rename it to make its semantics clearer or remove it and use getStatus() instead.
There was a problem hiding this comment.
Thanks for the question. isClosed() is not a shortcut for getStatus() == ProcessStatus.CLOSED — it is the predicate "this process was terminated out-of-band and must reject late task results", and it intentionally spans two statuses:
CLOSED— written byclose(false)(e.g. the table is released from the queue / the optimizer group changes) and by the partial-commit completion path incommit();KILLED— the terminal state this class already treats as "process is gone" inpoll()(status != KILLED && status != FAILED), and the recovery constructor restores the status from the persistedTableProcessMeta, so it can carry whatever the process framework persisted.
That is why simply removing it and comparing getStatus() at the call site does not express the intent:
- comparing
getStatus() == ProcessStatus.CLOSEDwould silently lose theKILLEDarm; - comparing
getStatus() == CLOSED || getStatus() == KILLEDjust re-creates this predicate inline, without a name.
For context, this is also exactly how the bug was introduced: #3257 migrated the old OptimizingProcess.Status check correctly (status == ProcessStatus.CLOSED), then #3486 changed the readers (poll(), the recovery check, isClosed()) from CLOSED to KILLED while leaving close() writing CLOSED. Since nothing in this class assigns KILLED, isClosed() has been permanently false and the guard in acceptResult() dead ever since. Keeping one named predicate for "results are no longer accepted" — instead of scattered raw status comparisons — is what prevents this kind of writer/reader drift from recurring.
On the naming — fair point that isClosed() covering KILLED can read as a CLOSED shortcut at first glance. I'd like to keep this hotfix minimal (just restoring the dead guard) and treat any rename (e.g. isTerminated()) as a follow-up if you think it is worth it, since it would touch the public OptimizingProcess interface.
2 participants
Brief change log
Make
OptimizingProcess.isClosed()recognize theCLOSEDstatus set byclose(), in addition toKILLED. This restores the guard against accepting late results for a closed optimizing process.How was this patch tested?
TestOptimizingQueue#testProcessCloseKeepsLastOptimizedSnapshotIdlocally with JDK 11 before creating this pull request.Documentation